Give more types to CachedOperation to avoid use of any#3342
Give more types to CachedOperation to avoid use of any#3342robertbrignull merged 3 commits intomainfrom
Conversation
koesie10
left a comment
koesie10
left a comment
There was a problem hiding this comment.
LGTM
I wanted to do it with one argument so you could do
CachedOperation<typeof this.getDefinitions>, but sadly I couldn't get this to work. The issue I hit was that the async function didn't like a return type ofReturnType<T>and claimed it wasn't aPromise. It's possible we could get this to work with one type arg, but the effort didn't seem worth it to me.
I was able to get this working using Awaited<ReturnType<T>>, but ran into some issues when using F extends (t: string, ...args: unknown[]) => Promise<unknown> as the type argument since it seems like the args: unknown[] doesn't work with typeof this.getDefinitions. I was able to work around this by using two type arguments, one of which is fully optional and just an implementation detail, but it makes it a little bit harder to understand intuitively. I've got this implementation in this branch. Please let me know if I should create a PR for this, I'm not entirely sure if this makes it clearer (although it does make the CachedOperation easier to use).
|
|
||
| /** | ||
| * A cached mapping from strings to value of type U. | ||
| * A cached mapping from args of type [string, S] to a value of type Promise<U>. |
There was a problem hiding this comment.
I think the original comment here was correct: only the string is used as the cache key, and any other arguments are not taken into account for the cache itself, only for calling the operation.
There was a problem hiding this comment.
Ah, thanks for pointing that out. I'll adjust the comment to be more accurate.
Thanks for having a go at this and working it out. I also don't know if this makes it clearer or not. I'm also not sure if my current PR makes things better. As your comments in that branch make clear, we're going to a lot of effort to remove the use of I'm also considering whether it's better to rewrite CachedOperation<T extends (t: string, ...args: any[]) => Promise<any>>and then tell eslint to ignore this one. It works but it still leads to doing Interestingly in terms of total lines of code (which obviously is not a perfect metric), this PR is the smallest. What I'm going to do right now is merge this PR because it fixes the problem at hand (including the error type) without creating too much code churn. This area is very easy to change so we could definitely do another PR to change things again. I'm not opposed to another PR, but I am wondering if it's best to just move on as it'll have little benefit 🤷🏼 |
2 participants
This PR removes the uses of
anyinCachedOperation. Unfortunately just replacing the use ofanywithunknownis not enough, but we can make it work if we add a type parameter for the function arguments.I've made it work by having two type arguments. One for the arguments and another for the return type. Hopefully this isn't too cumbersome. I did manage to remove one set of explicit type args from
template-provider.tsso now we only specify them once when declaring the variable and have them inferred when actually constructing theCachedOperationobject.I wanted to do it with one argument so you could do
CachedOperation<typeof this.getDefinitions>, but sadly I couldn't get this to work. The issue I hit was that the async function didn't like a return type ofReturnType<T>and claimed it wasn't aPromise. It's possible we could get this to work with one type arg, but the effort didn't seem worth it to me.Checklist
ready-for-doc-reviewlabel there.